Skip to content

Fix MC-868#9709

Closed
TreemanKing wants to merge 2 commits into
PaperMC:masterfrom
TreemanKing:FIX-MC-868
Closed

Fix MC-868#9709
TreemanKing wants to merge 2 commits into
PaperMC:masterfrom
TreemanKing:FIX-MC-868

Conversation

@TreemanKing
Copy link
Copy Markdown
Contributor

@TreemanKing TreemanKing commented Sep 11, 2023

Fixes MC-868
Adds a check if distance between minecart and detector rail is less than maximum distance of the centre of a block to the edge of a block (0.5)

Closes
#9698

Fixes MC-868
Adds a check if distance between minecart and detector rail is less than maximum distance of the centre of a blcok to the edge of a block (0.5)
@TreemanKing TreemanKing requested a review from a team as a code owner September 11, 2023 09:11
@vadcx
Copy link
Copy Markdown

vadcx commented Sep 11, 2023

Throughout the MC codebase to avoid the math.sqrt function (expensive) the distance checks are made on squared values.

If the classic formula is math.sqrt((x2-x1)²+(y2-y1)²) < limit

then removing sqrt on both sides of the equation leaves (x2-x1)²+(y2-y1)² < limit²

The ...distanceSquared function should be somewhere in vanilla. Please double-check me on the 0.5d² value.

Copy link
Copy Markdown
Member

@Machine-Maker Machine-Maker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks (possibly) expected behavior for the detector rail as it currently operates. I created the setup below and summoned a minecart at varying points on the track horizontally. In vanilla, I could spawn it on 3/10ths of the adjacent block and it would still trigger the detector rail. With this PR, the position has to be within the detector block x/z coord.

@TreemanKing
Copy link
Copy Markdown
Contributor Author

TreemanKing commented Sep 14, 2023

This breaks (possibly) expected behavior for the detector rail as it currently operates. I created the setup below and summoned a minecart at varying points on the track horizontally. In vanilla, I could spawn it on 3/10ths of the adjacent block and it would still trigger the detector rail. With this PR, the position has to be within the detector block x/z coord.

From testing that I did prior to posting the PR, I used the sqrt(2) as the maximum distance (centre to corner). When I had used this, the bug would still persist (considering a velocity of 0.7 or greater from memory), which I believe what current basic vanilla function uses. I think this is because the block is picked up by the detector rail on the corner is made by the rail converting the rail.

I believe the vanilla state is broken as a result as the block should only take the entities from the basic orthogonal directions as it can only face 4 directions, and to this that it's attempting to detect then interacting entity (entity within block), not centre to corner.

I do believe some redstone contraptions that use detector rails should be used as a test to see if any current redstone contraptions will break. Only thing I can think of is if the redstone is required to be on for that fraction of a second longer or a block as you mentioned.

@vadcx
Copy link
Copy Markdown

vadcx commented Sep 17, 2023

Treeman, did you consider the fix posted in the originial MC-868? Link to comment by FX - PR0CESS

Now I will mention why this should not be fixed xD

When changing this code you run into an issue with client rendering. Due to the client using interpolation, the client will render the minecart as if it's turning and then put it in the right place. So that issue should be solved first... which is not an easy issue to fix.

@TreemanKing
Copy link
Copy Markdown
Contributor Author

TreemanKing commented Sep 17, 2023

Treeman, did you consider the fix posted in the originial MC-868? Link to comment by FX - PR0CESS

Now I will mention why this should not be fixed xD

When changing this code you run into an issue with client rendering. Due to the client using interpolation, the client will render the minecart as if it's turning and then put it in the right place. So that issue should be solved first... which is not an easy issue to fix.

His solution checks a single co-ordinate/position.

@Owen1212055
Copy link
Copy Markdown
Member

Judging by MM's comment, it's probably not best for us to touch this since this may have implications in the whole technical world. This may also be behavior that is still used, so not sure if it's our place to patch something like this.

Additionally, I am not sure the code you are using will work on most setups due to it using awt classes.

Thank you for your contribution, regardless. :)

@Owen1212055 Owen1212055 closed this Jun 6, 2024
@TreemanKing TreemanKing deleted the FIX-MC-868 branch June 6, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

4 participants